Skip to content

test: print the .so under test, and name what PGC_SKIP_BUILD actually skips (#508 follow-up) - #513

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:feat/harness-so-fingerprint
Aug 8, 2026
Merged

test: print the .so under test, and name what PGC_SKIP_BUILD actually skips (#508 follow-up)#513
jdatcmd merged 1 commit into
commandprompt:mainfrom
ChronicallyJD:feat/harness-so-fingerprint

Conversation

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

Follow-up to #508, which fixed the harness reporting checks after a build that
failed. This makes the same class visible rather than fixing one instance of it.

pgc_setup now prints one line on every run:

-- .so: 8b58d7fdcb0d /usr/local/pg18a/lib/postgresql/pgcolumnar.so

Why a line nobody asked for

Three separate defects in a single session were a suite reporting checks against
a binary nobody had just built:

  • a compile error the harness did not check, which reported 19 green checks
    against a source file containing invalid C (fix: a suite must not report checks against the previously installed .so #508);
  • a PGC_SKIP_BUILD=1 run that skipped the install, not just the build, and
    exercised a guard-removed leftover from an earlier removal proof — three suites
    failing for a reason unrelated to the change;
  • objects from one major linked into another's .so, which surfaces as a cluster
    that will not start behind a message naming nothing.

Every one produced a plausible PASS/FAIL list. Every one is one line of md5sum
away from being obvious. The point is that it prints when nothing looks wrong,
because that is the state all three of those were in.

It also makes a red-on-change proof self-evidencing. Two arms reporting the same
hash have proved nothing whatever their check counts say, and today that is
something each of us has to remember to verify by hand.

PGC_SKIP_BUILD is named in the same change

The variable is not what it says. It reads as "skip the build" and means "skip
the build and the install, and test whatever is already installed" — correct
for the matrix, which installs once per major before setting it, and a trap for a
person who has just edited source and run make by hand. That is exactly how the
second defect above happened, so the harness now says it out loud.

Which removals were tried, and which one proved nothing

harness_selftest gains two checks: that the line exists, and — the one with
teeth — that what is installed matches what was just built, using the
build tree as an independent source rather than recomputing the installed hash
the same way twice. Recomputing it two ways proves only that md5sum is
deterministic.

Proved by reproducing the original incident rather than by deleting the check:

build a different binary, do not install it, run with PGC_SKIP_BUILD=1
  -> FAIL  the installed .so is the one this run built:
           got [no (installed 8b58d7fdcb0d, built 7564d4f138ed)] want [yes]

And the removal that proved nothing, which belongs here rather than buried in a
commit message.
My first attempt appended a C comment to a source file and
rebuilt, expecting a different binary. The hashes were identical — comments do not
survive to the object — the check passed, and it would have shipped labelled
"proved by removal". The line being justified is what revealed it.

The general form, which is worth more than this patch: a removal proof must
remove something the compiler, the planner or the runtime can observe.
A
comment; a fixture where two cases coincide; a guard nothing reaches. In each the
"before" arm equals the "after" by construction, and the check passes in both
while carrying the authority of a real proof.

Gate

Full matrix on 18 and 19. harness_selftest=PASS on both majors and not among
either job's skips.

PG19 reports temporal=FAIL in the container this ran in, which is its missing
btree_gist and which unmodified main fails identically there — measured, and
written up on #505. Happy to re-run on the bench host before merge if you would
rather see it without that caveat.

… skips

Three separate defects in one session were a suite reporting checks against a
binary nobody had just built:

  - a compile error the harness did not check, which reported 19 green checks
    against a source file containing invalid C (commandprompt#508);
  - a PGC_SKIP_BUILD=1 run that skipped the INSTALL, not just the build, and
    exercised a guard-removed leftover from an earlier removal proof -- three
    suites failing for a reason that had nothing to do with the change;
  - objects from one major linked into another's .so, which surfaces as a
    cluster that will not start behind a message naming nothing.

Every one produced a plausible PASS/FAIL list. Every one is one line of md5sum
away from being obvious. So pgc_setup now prints that line on every run, whether
or not anything looks wrong -- the point is precisely that nothing does.

It also makes a red-on-change proof self-evidencing. Two arms reporting the same
hash have proved nothing whatever their check counts say, and that is currently
something each person has to remember to verify by hand.

PGC_SKIP_BUILD is named in the same change because the variable is not what it
says: it reads as "skip the build" and means "skip the build AND the install, and
test whatever is already installed". That is correct for the matrix, which
installs once per major before setting it, and a trap for a person who has just
edited source and run make by hand -- which is exactly how the second defect
above happened.

harness_selftest gains two checks. The first is that the line exists. The second
is the one with teeth: it compares what is INSTALLED against what was just BUILT,
using the build tree as an independent source rather than recomputing the
installed hash the same way twice.

Proved by reproducing the original incident rather than by deleting the check:
build a genuinely different binary, do not install it, run with
PGC_SKIP_BUILD=1.

    installed 8b58d7fdcb0d, built 7564d4f138ed   -> FAIL, as it should

The first attempt at that proof appended a comment to a source file and rebuilt.
The binary was byte-identical, both hashes matched, and the check passed --
which the fingerprint line itself is what revealed. A perturbation that does not
perturb is not a proof, and this one says so out loud.

Refs commandprompt#508

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. Read it rather than approving the thing I asked for on the strength of having asked for it — that is the failure mode where a reviewer rubber-stamps their own suggestion.

The design is right and better than what I specified. Comparing installed against built, using the build tree as an independent source, can actually disagree; recomputing the installed hash two ways would only have proved md5sum is deterministic. And printing the line on every run rather than only on failure is the part that makes it useful — the defect it catches is invisible in a PASS/FAIL list, so a diagnostic that only appears when something already looks wrong would not have caught any of the three instances.

Naming what PGC_SKIP_BUILD actually skips is the half I would have under-weighted. "Not building AND NOT INSTALLING; whatever is already installed is what these checks measure" is the sentence that would have stopped me — I read the variable and reasoned about the build.

One non-blocking edge

harness_selftest under PGC_SKIP_BUILD=1 in a tree that was never built: _so_built is empty, so the check reports no (installed <hash>, built <none>) and reds.

Under the matrix that cannot happen — the runner builds into the builddir and only then sets the variable, so $PGC_SRCDIR/pgcolumnar.so is right there. It is reachable only by a person running the self-test standalone with PGC_SKIP_BUILD=1 from a clean checkout, which is a legitimate "test what is already installed" flow.

The honest outcome there is cannot tell, and this repository has a shape for that which is neither a pass nor a fail. A red is defensible — the harness genuinely cannot verify what it claims — but it fails as though the install were broken when nothing is. Your call whether it is worth distinguishing; I would not hold the PR for it.

The residual gap worth stating somewhere, since the check's name implies more than it can deliver: it detects install skew, not staleness. Built and installed can agree and both be old. That is still an enormous improvement over what we had, and it is exactly what caught me twice.

@jdatcmd
jdatcmd merged commit 4e8914b into commandprompt:main Aug 8, 2026
11 checks passed
ChronicallyJD pushed a commit that referenced this pull request Aug 9, 2026
…cts (#537)

Adversarial review by @ChronicallyJD, all three findings reproduced here before
being fixed. The first should have blocked the merge and did.

1. THE FIX WAS REMOVABLE WITHOUT A RED. Measured, not reasoned:

     delete the pgc_start_log_report call from pgc_setup  -> 70 checks, PASSED
     revert the summary path to a hardcoded pattern       -> 70 checks, PASSED
     assert a squatter unconditionally                    -> FAILED, 2 checks

   The 16 checks fed the three functions fixtures and proved their arithmetic.
   Nothing asserted the failure path calls any of them, so the entire
   contribution of this branch could be deleted and the suite still reported
   green. That is the same gap #538 found in #532's bench guards -- the same
   mistake, twice, the second time inside a fix for an issue about a message
   asserting something the code has not established.

   Six call-site checks now, over source text, which is the weaker kind and is
   labelled as such. The failure path needs a cluster that will not start, which
   this suite cannot stand up; a weak check on the call site beats none. Both
   greps are premised on the path still existing, or they approve nothing.

   Rows 1 and 2 above are now red. Row 3 already was.

2. pgc_fatal_pattern's docstring claimed "one definition, used by both the
   start-failure path and the summary path". pgc_start_log_report never called
   it; it hardcoded FATAL:|PANIC:. So the comment asserting the drift had been
   fixed was itself untrue, committed in the fix for #537.

   The divergence is right and argued below it. There are now two NAMED patterns,
   pgc_fatal_pattern and pgc_start_fatal_pattern, so the difference is greppable
   rather than two literals in two places, and a check asserts each path asks its
   own.

3. _sawforeign was sticky: set on any attempt, never cleared. One squatter on
   attempt 1 followed by seven genuine start failures printed the squatter
   verdict for all eight -- #537's own defect narrowed rather than removed, and
   reachable, since escaping a port collision is what the retry loop is for.

   It is a count now, with three cases: none, all, and the mixed one that a flag
   cannot express. The mixed case has its own check and fails when the old
   any-nonzero behaviour is restored.

One check of my own was wrong rather than the code: a grep for the inline verdict
matched two unrelated lines about the previously installed .so (#513). Tightened
to the start-failure text, with the reason recorded, since that is the same
prefix-matching trap this suite already guards for suite names.

harness_selftest 70 checks to 78. End-to-end re-verified with a genuinely broken
.so after the pattern change. Gate: PG17 assert 132 ran PASS, PG19 assert 137 ran
with only temporal, btree_gist absent from this container, identical on main.

Refs #537
ChronicallyJD pushed a commit that referenced this pull request Aug 9, 2026
…own rule (#545)

Review by @ChronicallyJD. Every one is the file not meeting the standard it
states, which is the only kind worth reporting in a change about how to argue.

CITED THE REMEDY AS THE DEFECT. The removal bullet listed #538 as a change that
shipped deletable. #538 is the change that CAUGHT it; the one that shipped
deletable was #532. Corrected, with #538 named as the catcher so the pair is
still findable.

THE PERSON-CLAIM SURVIVED ONE PARAGRAPH DOWN. I cut "Both authors had read this
line" from the removal bullet, said so in the commit message, and left "it
stayed quiet for two people writing fixes" in the trigger bullet, which asserts
the same unestablished thing in different words and is wrong the same way: both
changes were one author's. It now reads "for both changes above" and claims
nothing about anyone.

This is the finding worth keeping. A corrected sentence is not a corrected
belief, and the second instance is where you stop looking because the first one
felt like the fix. That is the same shape as the trigger bullet itself: knowing
the rule is not what makes it fire.

THE ONE UNCITED CLAIM. The loose-grep example carried no issue number while
#532, #537, #487 and zonemap_cost all do, in a section about not making
unverifiable statements. Now cites #537, and #513 for the message it collided
with.

Refs #545
ChronicallyJD pushed a commit that referenced this pull request Aug 9, 2026
…st (#545)

Proposed by @ChronicallyJD, and their wording rather than mine.

I had offered "a corrected sentence is not a corrected belief, and the second
instance is where you stop looking because the first one felt like the fix". True
as a diagnosis, and wrong as a bullet: its operative content is "notice that the
correction felt like completion", which is an instruction to notice harder. It
cannot be checked and it cannot fire, and it would have sat one paragraph below
"Consult it more carefully is not a fix" -- this section failing its own standard
a fourth time, in the sentence added to record the third.

The mechanical form earns its place: search for the same claim elsewhere, grep
for the phrasing you just removed. Ten seconds, names the tool, fails visibly
when skipped. Same properties as "can I delete this change and still be green".

The bullet then caught a fourth instance of its own class before this commit
landed, which is why the example in it is two failures rather than one. Verifying
every issue number in the added text -- which is what the bullet demands -- found
that the loose-grep example cited #513 for a message introduced by #508. The
previous commit had corrected exactly this defect, #538 cited where #532 belonged,
four lines away. The first correction felt like completion, again, in the edit
that recorded the first correction feeling like completion.

The psychology stays here in the history, where the story of how it happened is
findable, and the bullet carries only the part a reader can execute.

Refs #545
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants